Skip to content

Add support for lineLimit(_ limit: Int, reservesSpace: Bool) #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 28, 2025
Merged

Add support for lineLimit(_ limit: Int, reservesSpace: Bool) #161

merged 1 commit into from
May 28, 2025

Conversation

dfabulich
Copy link
Contributor

@dfabulich dfabulich commented Mar 24, 2025 •
edited
Loading

https://developer.apple.com/documentation/swiftui/view/linelimit(_:reservesspace:)

I've tested it with skiptools/skipapp-showcase#27 where it works on both Android and iOS.

  • REQUIRED: I have signed the Contributor Agreement
  • REQUIRED: I have tested my change locally with swift test
  • OPTIONAL: I have tested my change on an iOS simulator or device
  • OPTIONAL: I have tested my change on an Android emulator or device

@cla-bot cla-bot bot added the cla-signed label Mar 24, 2025
@dfabulich dfabulich mentioned this pull request Mar 24, 2025
Open
4 tasks
@dfabulich dfabulich force-pushed the text-linelimit-reservesspace branch 2 times, most recently from ec47d91 to 44699f6 Compare March 24, 2025 05:29
@dfabulich dfabulich marked this pull request as ready for review March 24, 2025 05:31
@dfabulich dfabulich force-pushed the text-linelimit-reservesspace branch from 44699f6 to 8cd4871 Compare March 24, 2025 05:40
aabewhite
aabewhite reviewed Mar 24, 2025
View reviewed changes
Copy link
Contributor

@aabewhite aabewhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Will take a look in the next couple days when I finish my current task. Off the bat, looks great, but I think I'd prefer to put lineLimit and reservesSpace together into a single Tuple (Pair in Kotlin) in the Environment. I try to group things together when it makes sense because I worry that eventually we're going to see a perf drag from so many Environment lookups

@dfabulich
Copy link
Contributor Author

I think a Tuple might be nicer, but I'm not sure it would make sense here, because .lineLimit is a built-in official environment value of SwiftUI, and it's an Int.

https://developer.apple.com/documentation/swiftui/environmentvalues/linelimit

The current released version of SkipUI can honor the .lineLimit environment variable today; it'll just work. If we changed the type of .lineLimit, it would break anyone who sets .environment(\.lineLimit, 2).

And if we don't change the type, then we just have to have a second environment variable. (Right?)

@aabewhite
Copy link
Contributor

Ah, I didn't notice that lineLimit is one of the exposed environment values. In that case you're absolutely right to keep them separate

aabewhite
aabewhite reviewed Mar 24, 2025
View reviewed changes
Copy link
Contributor

@aabewhite aabewhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request, otherwise looks good!

@dfabulich dfabulich force-pushed the text-linelimit-reservesspace branch from 8cd4871 to f63088c Compare May 28, 2025 03:45
@dfabulich
Copy link
Contributor Author

A couple of months later, I've resolved feedback; I think this is now ready to merge.

@marcprux marcprux merged commit 4bafa73 into skiptools:main May 28, 2025
2 checks passed
@marcprux
Copy link
Contributor

Looks great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabewhite aabewhite aabewhite left review comments

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dfabulich @aabewhite @marcprux